Skip to content

Use BLOB and consensus encoding for the entire header type#326

Merged
rustaceanrob merged 2 commits intomasterfrom
header-db-4-7
Apr 12, 2025
Merged

Use BLOB and consensus encoding for the entire header type#326
rustaceanrob merged 2 commits intomasterfrom
header-db-4-7

Conversation

@rustaceanrob
Copy link
Copy Markdown
Collaborator

@rustaceanrob rustaceanrob commented Apr 7, 2025

Building on #324, the header schema is tragically implemented considering Header is serializable and each component part can be recovered as required. I don't think the BLOB type can't be divided efficiently even if we know it will always be 80 bytes, but after this update the change seems noticeably faster on my end. Not even sure caching is urgent, it seems to pretty much rip. Almost comical how much simpler this is yet seemingly faster.

@rustaceanrob rustaceanrob force-pushed the header-db-4-7 branch 2 times, most recently from 812379b to a7cbc8a Compare April 10, 2025 08:37
@rustaceanrob rustaceanrob marked this pull request as ready for review April 11, 2025 12:03
@rustaceanrob
Copy link
Copy Markdown
Collaborator Author

Alright, instead of purely going on "trust me bro" I did a quick benchmark. The header sync over signet with the new and old schema both took exactly 93 seconds. I checked I was on the proper branch for each and I was, so this change is neutral apparently. To make the case for why the schema should change, when migrating to a new SQL schema we can always add optional columns but we can't take them away without nuking the user's headers


pub(crate) const DEFAULT_CWD: &str = ".";
pub(crate) const DATA_DIR: &str = "data";
pub(crate) const DATA_DIR: &str = ".light_client_data";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be called kyoto or something? Might be a little vague for a user to track down what is writing to this (unless I am missing some namespace). The . also makes this a little less cross platform friendly, since I don't think it's a windows convention to hide files/directories like that. On unix-like systems, it might be nice to use some XDG conventions too.

Copy link
Copy Markdown
Collaborator Author

@rustaceanrob rustaceanrob Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't aware . doesn't play nice with Windows. I don't think we need to go full XDG because the intention is this directory stores the bitcoin/signet/testnet data for the light client in an arbitrary location. The developer is then responsible to throw that into an appropriate spot (different for iOS/Ubuntu etc.).

My opinion against using kyoto names is the user somehow has to know their wallet "X" uses a repository called "kyoto" on Github to make sense of that directory. Anything with "light client" at least hints that there is data stored that underpins their wallet

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw I think XDG integration would be pretty light touch, like just checking for a XDG_DATA_HOME env var and using it if it's there. But I see the point of the wallet app doing that and then passing a path to its internal kyoto to write to (e.g. $HOME/.local/share/fancy-wallet/kyoto.

And if the intention is for the outer wallet to namespace this on the filesystem (the $HOME/.local/share/fancy-wallet/ part), I guess the name of this directory is less important since should really only be cared about by the wallet developer. Thinking from their perspective, I guess I wouldn't care much what it's called as long as I wasn't surprised at some point "wtf is this random data dir".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped the . prefix in 008cb2b

There's not much of a reason to hide this directory anyway, all the data is public via p2p gossip.

And if the intention is for the outer wallet to namespace this on the filesystem

Yeah, this is the intention. The developer is responsible for where they would like this to go. This crate shouldn't make any opinions about absolute paths.

Copy link
Copy Markdown
Collaborator

@nyonson nyonson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 358bb1d

@rustaceanrob rustaceanrob merged commit f9539ae into master Apr 12, 2025
12 checks passed
@rustaceanrob rustaceanrob deleted the header-db-4-7 branch April 12, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants